Skip to content

Use RenderStartup for bevy_solari. #19918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use RenderStartup for bevy_solari. #19918

wants to merge 1 commit into from
+178 −112

Conversation

andriyDev
Copy link
Contributor

Objective

Solution

  • Convert any resource mutations into systems (including resource init).
  • Add the render graph nodes from systems.
  • Create a SolariSystems system set that all systems belong to.
  • Check if the required features are present in a system and insert a resource if they are.
  • Add a run condition to SolariSystems to only execute systems if the resource is present.

Testing

  • Ran the solari example, and it still works.

@andriyDev
Copy link
Contributor Author

This PR is based off of #19912 so we don't have to restructure how we add render graph nodes. I will wait for that before sending it out for review.

@andriyDev andriyDev added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 2, 2025
@andriyDev andriyDev force-pushed the solari-conditional branch from 2b369a7 to a770f7d Compare July 2, 2025 17:41
@andriyDev andriyDev marked this pull request as ready for review July 2, 2025 17:45
@andriyDev andriyDev added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 2, 2025
@andriyDev andriyDev requested review from atlv24, IceSentry and JMS55 July 2, 2025 17:45
@JMS55
Copy link
Contributor

JMS55 commented Jul 3, 2025 •
edited
Loading

I really don't like this solution :/. It really complicates the solari code.

Perhaps we could make a generic version of this that multiple plugins could use?

@andriyDev
Copy link
Contributor Author

@JMS55 I'm not sure I agree that this complicates the solari code significantly. The only part that isn't specific to solari code is I think the fact that we have to configure the system sets 3 times.

I tried drafting this up and it's quite gross 0da4a59

In theory the conditional_render_set function would exist in bevy_render or something. But this leads to a lot of complexity that is just hidden. And I suspect it's going to be more difficult to debug because of these generic system sets.

To me, this seems like a lot of complexity that makes it less clear what's going on and why.

Looking through our code, here are these cases with finish impls that check some resources before inserting stuff.

  • crates\bevy_core_pipeline\src\oit\resolve\mod.rs
  • crates\bevy_pbr\src\atmosphere\mod.rs
  • crates\bevy_pbr\src\meshlet\mod.rs
  • crates\bevy_pbr\src\render\gpu_preprocess.rs
  • crates\bevy_pbr\src\render\mesh.rs
  • crates\bevy_pbr\src\ssao\mod.rs

Feel free to investigate, but the TL;DR for what these conditionals access is:

  • features from RenderDevice
  • limits from RenderDevice
  • "downlevel_capabilities" from RenderAdapter
  • allowed usages for textures in RenderAdapter
  • is_available from GpuPreprocessingSupport.

That's quite a mix of things we access, so it's not as simple as adding a run condition for features or something (plus I want to make the run conditions themselves as cheap as possible).

@IceSentry
Copy link
Contributor

IceSentry commented Jul 13, 2025 •
edited
Loading

To me this doesn't look particularly that complicated. It's a bit more setup code, but it also makes things more explicit which I think is valuable. I like the idea of a HasRequiredFeatures resource for a run condition. With that said, I'm not entirely sure the system sets is strictly needed? It's only used in a few places and it only exists for the purpose of the run condition but I feel like adding the run condition directly where it's needed might be more explicit and lead to overall a bit less code? We can always add back the system set in the future if we need more complex scheduling.

IceSentry
IceSentry reviewed Jul 13, 2025
View reviewed changes
crates/bevy_solari/src/pathtracer/mod.rs
// Making this a system that runs at RenderStartup allows a run condition to check for required
// features first.
fn add_solari_pathtracing_render_graph_nodes(world: &mut World) {
world
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should support the slightly nicer .add_render_graph_node directly on the RenderGraph and then query that. It's a bit weird to get the entire world just for that. It would be more explicit that way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to do in this PR to be clear, but as a follow up or before this PR merges it would be worth looking into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I was definitely more focused on keeping the diff small. One advantage of this heavy-handed approach is we have to do significantly less error handling - all the error handling and logging is already in add_render_graph_node.

I'd be down to do that as a followup though.

IceSentry
IceSentry reviewed Jul 13, 2025
View reviewed changes
crates/bevy_solari/src/lib.rs
/// are enabled.
///
/// Now systems can do a cheap check for if the resource exists.
fn check_solari_has_required_features(mut commands: Commands, render_device: Res<RenderDevice>) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused where is this system actually added to RenderStartup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally goofed haha. I added the system now!

@andriyDev andriyDev force-pushed the solari-conditional branch from a770f7d to fcdc082 Compare July 13, 2025 16:55
@andriyDev
Copy link
Contributor Author

Due to the refactors from SolariPlugin to a plugin group, I made a new plugin just focused on the configure_sets stuff, and added that to the plugin group.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IceSentry IceSentry IceSentry left review comments

@atlv24 atlv24 Awaiting requested review from atlv24

@JMS55 JMS55 Awaiting requested review from JMS55

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Rendering
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andriyDev @JMS55 @IceSentry